Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(header): properly support the Icon interface #3421

Merged
merged 1 commit into from
Jan 30, 2025
Merged

feat(header): properly support the Icon interface #3421

merged 1 commit into from
Jan 30, 2025

Conversation

LucyChyzhova
Copy link
Contributor

Now the consumer can specify color and backgroundColor directly on the icon. The styles code are however backwards compatible, and still support the old custom CSS props of --header-icon-color and --header-icon-background-color. In case any consumer has specified the CSS props, they will get the same result as before. But we removed the props from the documentations, since we want the consumers to use the Icon interface instead of adding styles.

Review:

  • Commits are atomic
  • Commits have the correct type for the changes made
  • Commits with breaking changes are marked as such

Browsers tested:

(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)

Windows:

  • Chrome
  • Edge
  • Firefox

Linux:

  • Chrome
  • Firefox

macOS:

  • Chrome
  • Firefox
  • Safari

Mobile:

  • Chrome on Android
  • iOS

@LucyChyzhova LucyChyzhova requested a review from Kiarokh January 30, 2025 10:33
@LucyChyzhova LucyChyzhova self-assigned this Jan 30, 2025
@LucyChyzhova LucyChyzhova requested a review from a team as a code owner January 30, 2025 10:35
@github-actions GitHub Actions
Copy link

Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3421/

Kiarokh
Kiarokh previously approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@Kiarokh Kiarokh left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the correct way to move forward, for all components, and get rid of all those silly and old CSS props. But I cannot merge this PR, it needs some blessings from @adrianschmidt @jgroth

@Kiarokh Kiarokh self-assigned this Jan 30, 2025
@adrianschmidt adrianschmidt self-assigned this Jan 30, 2025
@adrianschmidt
Copy link
Contributor

I'll have a look 👍

@LucyChyzhova @adrianschmidt
feat(header): properly support the Icon interface
75f01bf 
Now the consumer can specify `color` and `backgroundColor` directly
on the `icon`. The styles code are however backwards compatible,
and still support the old custom CSS props of `--header-icon-color`
and `--header-icon-background-color`. In case any consumer has
specified the CSS props, they will get the same result as before.
But we removed the props from the documentations, since we
want the consumers to use the `Icon` interface instead of adding
styles.
@adrianschmidt
Copy link
Contributor

The changes to etc/lime-elements.api.md completely disappeared when I ran npm run api:update, so I don't know what weird state you were in to get all the changes you did.

It seems like once that file has been touched in a PR, the PR requires a review from a maintainer, even if those changes are then removed from the PR. So if you want to avoid having to wait for a maintainer for no good reason, keep an eye on what you push 😊

adrianschmidt
adrianschmidt approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@adrianschmidt adrianschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubberstamp

@adrianschmidt adrianschmidt merged commit a70b6d0 into main Jan 30, 2025
12 checks passed
@adrianschmidt adrianschmidt deleted the header-icon-color branch January 30, 2025 11:57
@lime-opensource
Copy link
Collaborator

🎉 This PR is included in version 37.81.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrianschmidt adrianschmidt adrianschmidt approved these changes

@Kiarokh Kiarokh Kiarokh left review comments

Assignees

@adrianschmidt adrianschmidt

@Kiarokh Kiarokh

@LucyChyzhova LucyChyzhova

Labels
released 🦄✨ yolo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LucyChyzhova @adrianschmidt @lime-opensource @Kiarokh